Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alloc: fix memory heap initialization #541

Merged
merged 1 commit into from
Nov 8, 2018

Conversation

libinyang
Copy link
Contributor

@libinyang libinyang commented Nov 5, 2018

This patch fixes the bug of runtime & buffer memory heap initialization and
sets the correct base for each map.
This is a generic fix and It was verified to fix the issues below:
thesofproject/linux#253
#539 duplicate of linux #253

Signed-off-by: Libin Yang libin.yang@intel.com

@libinyang
Copy link
Contributor Author

@lgirdwood
@tlauda

Hi all,

This is a patch to fix the bug of memory initialization in FW.

@wenqingfu
Copy link

Is there a corresponding issue number of the "bug"? @libinyang

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain the bug and the fix in detail in the commit message (otherwise it;s really hard to follow what you are fixing and why).

@libinyang
Copy link
Contributor Author

@wenqingfu
@lgirdwood
@mmaka1
@tlauda

This patch is made for the bug #539. However, as the memory issue should be very general, I believe this patch may fix some other potential unstable issues because of the memory allocation.

This bug is the member base of memmap.runtime[] map and memmap.buffer[] map is set wrong.

@libinyang
Copy link
Contributor Author

Patch updated by adding more comments in the code.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe what is broken with existing code and how your changes fix this in the commit message otherwise this cant be applied (even if it's correct). I've spent 20 minutes working out what is wrong with the current code that could have been avoided if you had explained in the commit message.

src/lib/alloc.c Outdated
int i;
int j;
int k;
int i, j;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please follow coding style of one variable per line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please follow coding style of one variable per line.

Thanks for point it out. I didn't realize we have such coding style in sof project.

@@ -626,21 +624,19 @@ void init_heap(struct sof *sof)
/* initialise buffer map */
for (i = 0; i < PLATFORM_HEAP_BUFFER; i++) {
heap = &memmap.buffer[i];

for (j = 0; j < heap->blocks; j++) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep the new line

@@ -649,21 +645,19 @@ void init_heap(struct sof *sof)
/* initialise runtime map */
for (i = 0; i < PLATFORM_HEAP_RUNTIME; i++) {
heap = &memmap.runtime[i];

for (j = 0; j < heap->blocks; j++) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep new line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for this and upper review comments.

This patch fixes the bug of runtime & buffer memory heap initialization,
and sets the correct base address for each map.

Signed-off-by: Libin Yang <libin.yang@intel.com>
@plbossart
Copy link
Member

this change makes issue thesofproject/linux#253 go away, can we please prioritize reviews and integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants